Fix two crashes in the default DNS forward/cache path - #21784
Conversation
Rex::Proto::DNS::Server#default_dispatch_request crashed with a NoMethodError the moment it had to finalize an empty response, which happens on any query that misses the cache and comes back from the resolver with no answers - an out-of-scope query being forwarded, for instance, or any query with a genuinely empty result. The exception was uncaught, so it killed the listener thread and stopped the whole server. Two mistakes, both on code Packet.encode_drb documents as returning a Dnsruby::Message: - req.header.rCode= does not exist on Dnsruby::Header; the real setter is rcode= (lowercase). The class does define an rCode-cased method elsewhere as a getter alias, which is presumably what led to the wrong casing here. - req.data does not exist on Dnsruby::Message either; the real serializer is #encode. Packet.encode_raw, a few lines above in the same file, already handles this correctly by checking respond_to?(:data) for a legacy Net::DNS::Packet and falling back to #encode otherwise - default_dispatch_request just used the wrong branch of that same distinction. No existing spec covered this path, so both bugs shipped and stayed live. Reproduced against a real client. Fixed and added coverage for the empty answer case that crashed.
There was a problem hiding this comment.
Pull request overview
This pull request fixes two NoMethodError crashes in Rex::Proto::DNS::Server#default_dispatch_request that could terminate the DNS listener thread when finalizing and returning an empty forwarded response, and adds an RSpec regression test for that path.
Changes:
- Corrects the Dnsruby header setter from
rCode=torcode=. - Replaces the nonexistent
Dnsruby::Message#datacall with the correct serializer#encode. - Adds an RSpec spec that exercises the “forwarded response has no answers” path and asserts the server returns a valid encoded DNS reply without raising.
Impact Analysis:
- Blast radius: medium — affects any Metasploit code paths using
Rex::Proto::DNS::Server(including default forward/cache handling); downstream consumers are DNS-server-backed modules and listeners. - Data and contract effects: no schema/payload contract changes beyond producing a response instead of crashing; fixes runtime behavior from “thread dies” to “sends encoded DNS response”.
- Rollback and test focus: rollback is straightforward (revert); test focus should be DNS server forwarding with empty responses (covered by the added spec) and a quick sanity check that normal answered forwarding still works.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lib/rex/proto/dns/server.rb | Fixes two incorrect Dnsruby API calls that caused crashes when finalizing/sending empty forwarded responses. |
| spec/lib/rex/proto/dns/server_spec.rb | Adds regression coverage for the previously-crashing empty-forwarded-response path. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
jheysel-r7
moved this from In Progress
to What about Second Review?
in Metasploit Kanban
jheysel-r7
left a comment
jheysel-r7
left a comment
There was a problem hiding this comment.
Great catch @Pushpenderrathore. This has been tested as a part of the ESC8 Kerberos Relaying work.
Verification
To whomst-ever does a second review please see the following:
The setter in dnsruby's header.rb is indeed all lower case:
https://github.com/alexdalitz/dnsruby/blob/master/lib/dnsruby/message/header.rb#L86-L88
And there is no #data method in the Message class it is #encode as seen here:
https://github.com/alexdalitz/dnsruby/blob/master/lib/dnsruby/message/message.rb#L541-L561
github-project-automation
Bot
moved this from What about Second Review?
to In Progress
in Metasploit Kanban
Release NotesThis pull request fixes two uncaught exceptions in the default DNS forward/cache path that would silently kill the listener thread when finalizing an empty response. The crashes were caused by incorrect method calls to the Dnsruby library, which have been updated to use the proper rcode= setter and #encode serializer. As a result, the DNS server now correctly handles and forwards out-of-scope queries without terminating. |
Description
Rex::Proto::DNS::Server#default_dispatch_requestcrashes the moment it has to finalize an empty response. That happens on any query that misses the cache and gets forwarded to the resolver with no answer coming back, which in practice includes any out-of-scope query being forwarded through unchanged, such as the PTR reverse lookup a real client sends before its actual query. The exception is uncaught, so it kills the listener thread and stops the whole server, silently, on the very first such query.Two mistakes in the same two lines, both against
Dnsruby::Message, the typePacket.encode_drbdocumentsreqas being:req.header.rCode=does not exist onDnsruby::Header; the setter isrcode=(lowercase). The same directory already uses the correct setter and reader —lib/rex/proto/dns/packet.rb:155and:158setpacket.header.rcode = Dnsruby::RCode::NXDOMAIN/NOERROR, and:157reads viaget_header_rcode— so this fix just aligns the crashing path with the file's own established convention.req.datadoes not exist onDnsruby::Message. The real serializer is#encode.Packet.encode_raw, a few lines above in this same file, already gets this right:default_dispatch_requestjust used the wrong branch of the samedata/encodedistinction this file already knows how to make.No spec covered this path, so both shipped and stayed live.
Live reproduction
I hit this while building an IPv6 DNS-takeover module. Once the crash was fixed I could see it happening: with only the first fix applied, the second line raised immediately on the next real query.
With both fixed, the same real client's query is handled and forwarded correctly, and the server survives.
Testing
Added
spec/lib/rex/proto/dns/server_spec.rb, covering the path that crashed: a forwarded query whose response carries no answers. Confirmed the new spec fails against the pre-fix code with the exact errors above, and passes with the fix.Breaking Changes
None. Both lines were unreachable without raising; nothing depended on the old behavior.